BUG: issues with encoding and null field updates #60
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThree repository update methods were changed to include None-valued fields in serialized payloads; InfinityEncoder gained an init, had its default parameter renamed, and no longer invokes Flight post-processing; EnvironmentSimulation.max_expected_height type widened to float; RocketPy dependency switched to a git-based develop branch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Repo as Repository
participant Model as PydanticModel
Client->>Repo: update_*_by_id(id, payload_model)
Repo->>Model: payload_model.model_dump(exclude_none=False)
note right of Repo #E8F0FF: serialized payload now includes None fields
Repo->>Repo: update_by_id(id, serialized_payload)
Repo-->>Client: update result
sequenceDiagram
autonumber
participant Caller
participant Enc as InfinityEncoder
participant Flight
participant Parent as JSONEncoder
Caller->>Enc: encode(obj: Flight)
Enc->>Flight: access obj._Flight__evaluate_post_process
note right of Enc #E8F0FF: attribute read only — post-process not invoked
alt Flight handled
Enc->>Flight: read fields (including None)
Enc-->>Caller: serialized JSON
else fallback
Enc->>Parent: parent.default(obj)
Parent-->>Enc: fallback result
Enc-->>Caller: serialized JSON
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Points to check during review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/repositories/motor.py (1)
29-32: Same concern as in flight.py: verify None-value handling.This change mirrors the one in
src/repositories/flight.py. Ensure that including None-valued fields in the update payload aligns with the intended update semantics and doesn't cause unintended data overwrites.src/repositories/rocket.py (1)
29-32: Same concern as in flight.py: verify None-value handling.This change mirrors the one in
src/repositories/flight.py. Ensure that including None-valued fields in the update payload aligns with the intended update semantics and doesn't cause unintended data overwrites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/repositories/flight.py(1 hunks)src/repositories/motor.py(1 hunks)src/repositories/rocket.py(1 hunks)src/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/repositories/rocket.py (6)
src/models/rocket.py (3)
UPDATED(41-42)RocketModel(13-63)DELETED(45-46)src/routes/rocket.py (1)
update_rocket(56-68)tests/unit/test_routes/test_rockets_route.py (1)
test_update_rocket(357-365)src/controllers/flight.py (1)
update_rocket_by_flight_id(47-66)src/routes/flight.py (1)
update_flight_rocket(140-155)tests/unit/test_routes/test_flights_route.py (1)
test_update_rocket_by_flight_id(190-199)
src/repositories/flight.py (3)
src/routes/flight.py (1)
update_flight(58-70)tests/unit/test_routes/test_flights_route.py (5)
test_update_flight_invalid_input(212-216)test_update_environment_by_flight_id(176-187)test_update_flight_by_id(166-173)test_update_rocket_by_flight_id(190-199)test_update_flight_not_found(219-225)src/controllers/flight.py (2)
update_rocket_by_flight_id(47-66)update_environment_by_flight_id(25-44)
src/repositories/motor.py (3)
src/routes/motor.py (1)
update_motor(56-68)src/views/motor.py (2)
MotorView(76-77)MotorSimulation(7-73)tests/unit/test_routes/test_motors_route.py (3)
test_update_motor(291-298)test_update_motor_not_found(308-316)test_update_motor_invalid_input(301-305)
src/utils.py (1)
src/views/flight.py (1)
FlightSimulation(9-140)
🪛 GitHub Actions: Pylint
src/utils.py
[error] 78-78: pylint: C0303 trailing whitespace detected.
[error] 83-83: pylint: C0303 trailing whitespace detected.
[error] 89-89: pylint: C0303 trailing whitespace detected.
[error] 92-92: pylint: C0303 trailing whitespace detected.
[error] 95-95: pylint: C0303 trailing whitespace detected.
[error] 98-98: pylint: W0612 Unused variable 'e' (unused-variable).
🪛 Ruff (0.13.3)
src/utils.py
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
94-94: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
98-98: Do not catch blind exception: Exception
(BLE001)
98-98: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
There was a problem hiding this comment.
Pull Request Overview
This PR addresses encoding issues and null field update problems in the flight simulation system. The main issue was that the _Flight__evaluate_post_process() method was corrupted as a numpy array instead of a callable method, causing encoding failures.
- Added robust error handling and corruption detection for the
_Flight__evaluate_post_processmethod in the JSON encoder - Changed repository update methods to include null fields instead of excluding them
- Implemented fallback mechanisms for encoding errors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/utils.py | Added comprehensive error handling and corruption detection for Flight object encoding |
| src/repositories/rocket.py | Changed model dump to include null fields during updates |
| src/repositories/motor.py | Changed model dump to include null fields during updates |
| src/repositories/flight.py | Changed model dump to include null fields during updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/utils.py
Outdated
| try: | ||
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) |
There was a problem hiding this comment.
After deleting the corrupted attribute, immediately accessing it again may not restore the original method. The cached_property mechanism might not automatically recreate the method. Consider using the class definition to access the original property descriptor.
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) | |
| # Access the property via the class descriptor to restore it correctly | |
| restored_method = type(obj)._Flight__evaluate_post_process.__get__(obj, type(obj)) |
There was a problem hiding this comment.
Wdyt @aasitvora99 ?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/utils.py
Outdated
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) | ||
| if restored_method and callable(restored_method): |
There was a problem hiding this comment.
Arguably this is a tautology.
If it is callable it will always pass the first check.
Maybe just reduce it to if callable(obj)
There was a problem hiding this comment.
reverted my changes back to normal, unblocked by bressan on call.
src/utils.py
Outdated
| except Exception as fix_error: | ||
| logger.error(f"Error fixing _Flight__evaluate_post_process: {fix_error}") | ||
|
|
||
| elif evaluate_post_process is not None and callable(evaluate_post_process): |
There was a problem hiding this comment.
Same here.
Only callable check is enough
There was a problem hiding this comment.
reverted my changes back to normal, unblocked by bressan on call.
src/utils.py
Outdated
| try: | ||
| delattr(obj, '_Flight__evaluate_post_process') | ||
|
|
||
| restored_method = getattr(obj, '_Flight__evaluate_post_process', None) |
There was a problem hiding this comment.
Wdyt @aasitvora99 ?
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils.py (2)
src/views/flight.py (1)
FlightSimulation(9-140)src/controllers/flight.py (1)
FlightController(12-108)
🪛 GitHub Actions: Pylint
src/utils.py
[error] 78-78: E0001: Parsing failed: 'expected an indented block after 'try' statement on line 77 (src.utils, line 78)'
🪛 Ruff (0.13.3)
src/utils.py
77-78: Expected an indented block after try statement
(invalid-syntax)
78-78: Expected except or finally after try block
(invalid-syntax)
79-79: Unexpected indentation
(invalid-syntax)
82-82: Unexpected indentation
(invalid-syntax)
89-89: Expected except or finally after try block
(invalid-syntax)
89-89: Expected an identifier, but found a keyword 'except' that cannot be used here
(invalid-syntax)
89-89: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
89-89: Expected a statement
(invalid-syntax)
89-90: Expected an expression
(invalid-syntax)
91-91: Unexpected indentation
(invalid-syntax)
91-91: Expected a statement
(invalid-syntax)
91-91: Expected a statement
(invalid-syntax)
91-92: Expected an expression
(invalid-syntax)
92-92: Unexpected indentation
(invalid-syntax)
94-94: unindent does not match any outer indentation level
(invalid-syntax)
94-94: Invalid annotated assignment target
(invalid-syntax)
94-95: Expected an expression
(invalid-syntax)
95-95: Unexpected indentation
(invalid-syntax)
97-97: unindent does not match any outer indentation level
(invalid-syntax)
97-97: Expected a statement
(invalid-syntax)
97-97: Expected a statement
(invalid-syntax)
97-98: Expected an expression
(invalid-syntax)
99-99: Unexpected indentation
(invalid-syntax)
100-100: unindent does not match any outer indentation level
(invalid-syntax)
102-102: unindent does not match any outer indentation level
(invalid-syntax)
102-102: Expected a statement
(invalid-syntax)
102-102: Only single target (not tuple) can be annotated
(invalid-syntax)
102-103: Expected an expression
(invalid-syntax)
103-103: Unexpected indentation
(invalid-syntax)
🔇 Additional comments (1)
src/utils.py (1)
100-104: Verify the indentation and control flow for solution handling.After fixing the indentation issues, ensure that lines 100-103 (the try-except block for solution retrieval) are properly indented within the
if isinstance(obj, Flight):block. The early return on line 103 should prevent reaching line 104 when the exception occurs, but the current broken indentation makes this unclear.As noted in previous comments, if the fallback on line 103 executes,
solutionwill be undefined. However, the earlyreturnshould prevent reaching line 104. Confirm the intended control flow once indentation is corrected.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 44a2ebc.
…/Infinity-API into bug/simulate-forward-fixes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils.py (1)
57-58: Remove unnecessary__init__override.The
__init__method only delegates to the parent class without adding any custom initialization. Python automatically calls the parent's__init__when not overridden, making this override redundant.Apply this diff to remove the unnecessary override:
- def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - def default(self, obj):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pylint
src/utils.py
[error] 60-60: Pylint: W0237 - Parameter 'o' has been renamed to 'obj' in overriding InfinityEncoder.default method (arguments-renamed).
[error] 78-78: Pylint: W0104 - Statement seems to have no effect (pointless-statement).
🪛 Ruff (0.13.3)
src/utils.py
78-78: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (2)
src/utils.py (2)
60-60: Parameter rename improves clarity.Renaming the parameter from
otoobjis more descriptive and consistent with the method body. The Pylint warning (W0237) about parameter renaming in an overriding method is expected and acceptable here.
78-78: Clarify intent of private attribute access on Flight
The expressionobj._Flight__evaluate_post_processmerely reads a name-mangled member without calling or using it. Confirm whether this is meant to trigger a cached evaluation (and exists on the externalFlightclass), document that side effect, handle corruption, or remove it:
- Document as
# Access cached_property to trigger evaluation before serialization- Or detect/log corruption: check if it’s an
np.ndarrayand warn- Or remove the line if it serves no purpose
|
@GabrielBarberini ran lint, format and tests |
| gunicorn | ||
| uvicorn | ||
| rocketpy | ||
| git+https://github.com/RocketPy-Team/RocketPy.git@develop |
There was a problem hiding this comment.
so we'll pin to rocketpy develop branch from now on ?
| async def update_motor_by_id(self, motor_id: str, motor: MotorModel): | ||
| await self.update_by_id( | ||
| motor.model_dump(exclude_none=True), data_id=motor_id | ||
| motor.model_dump(exclude_none=False), data_id=motor_id |
| ) | ||
| if isinstance(obj, Flight): | ||
| obj._Flight__evaluate_post_process() | ||
| obj._Flight__evaluate_post_process |
There was a problem hiding this comment.
We don't want to evaluate post process?
Linter is still failing, we need to either argue around dismissing this formally under the pyproject toml or inline (prferred) with a reasonable excuse or fix this |
Some fields from encoding was causing issues in the
_Flight__evaluate_post_process()method. Made a small hotfix for now to remove the bad actors that could not be converted.Summary by CodeRabbit
New Features
Bug Fixes
Chores